Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add poco basic and full test case #2928

Merged
merged 1 commit into from
Nov 9, 2024
Merged

Add poco basic and full test case #2928

merged 1 commit into from
Nov 9, 2024

Conversation

ziyue-101
Copy link
Collaborator

@ziyue-101 ziyue-101 commented Oct 16, 2024 •
edited
Loading

Change description

Add test coverage for standalone PoCo

Tests you have done

  • Run make ready-pr to ensure this PR is ready for review.
  • Perform necessary E2E testing for changed resources.

@ziyue-101
Copy link
Collaborator Author

/hold

@ziyue-101 ziyue-101 changed the title debug wip debug poco test wip Oct 16, 2024
@ziyue-101
Copy link
Collaborator Author

ziyue-101 commented Oct 16, 2024 •
edited
Loading

/hold cancel

@ziyue-101 ziyue-101 changed the title debug poco test wip debug poco mock test wip Oct 16, 2024
@ziyue-101 ziyue-101 changed the title debug poco mock test wip Add poco basic and full test case Nov 5, 2024
@ziyue-101
Copy link
Collaborator Author

/assign @acpana

@ziyue-101
Copy link
Collaborator Author

/unhold

acpana
acpana reviewed Nov 5, 2024
View reviewed changes
Copy link
Collaborator

@acpana acpana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple small questions. Could we also run the dynamic/ post submit test here and paste the results ?

...icpocogkehubfeaturemembership/_generated_object_basicpocogkehubfeaturemembership.golden.yaml Outdated Show resolved Hide resolved
mockgcp/generated/mockgcp/cloud/gkehub/v1beta/feature.pb.go Show resolved Hide resolved
@ziyue-101 ziyue-101 force-pushed the add-poco-test branch 2 times, most recently from 79a557c to f2126a2 Compare November 5, 2024 22:35
@ziyue-101
Copy link
Collaborator Author

Couple small questions. Could we also run the dynamic/ post submit test here and paste the results ?

recorded logs in https://docs.google.com/document/d/1858ckSv5AokAXvDiwfVZ6ES6e9l_LFn6V6-5XA2CtPI/edit?resourcekey=0-5FdU0kfwjrTJMsCXQ6ALSw&tab=t.0

@acpana
Copy link
Collaborator

acpana commented Nov 6, 2024

thanks for adding this test case 💯

/lgtm
/approve

@google-oss-prow google-oss-prow bot added the lgtm label Nov 6, 2024
@acpana
Copy link
Collaborator

acpana commented Nov 6, 2024

cc @yuwenma / @justinsb / @maqiuyujoyce

could one of you 👍🏼 this?

it bumps up the mockgcp proto and adds a test for the poco field in the GKEHubFeatureMemebership resource. Note the dynamic (post submit) test seems to pass too 💯

maqiuyujoyce
maqiuyujoyce reviewed Nov 6, 2024
View reviewed changes
Copy link
Collaborator

@maqiuyujoyce maqiuyujoyce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall lgtm. No comment is really a blocker except for the potential racing condition issue.

...ic/gkehub/v1beta1/gkehubfeaturemembership/basicpocogkehubfeaturemembership/dependencies.yaml Show resolved Hide resolved
...ic/gkehub/v1beta1/gkehubfeaturemembership/basicpocogkehubfeaturemembership/dependencies.yaml Outdated Show resolved Hide resolved
...ic/gkehub/v1beta1/gkehubfeaturemembership/basicpocogkehubfeaturemembership/dependencies.yaml Show resolved Hide resolved
...ic/gkehub/v1beta1/gkehubfeaturemembership/basicpocogkehubfeaturemembership/dependencies.yaml Show resolved Hide resolved
...ic/gkehub/v1beta1/gkehubfeaturemembership/basicpocogkehubfeaturemembership/dependencies.yaml Outdated Show resolved Hide resolved
...ata/basic/gkehub/v1beta1/gkehubfeaturemembership/fullpocogkehubfeaturemembership/create.yaml Show resolved Hide resolved
@google-oss-prow google-oss-prow bot removed the lgtm label Nov 8, 2024
maqiuyujoyce
maqiuyujoyce reviewed Nov 9, 2024
View reviewed changes
Copy link
Collaborator

@maqiuyujoyce maqiuyujoyce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@google-oss-prow google-oss-prow bot added the lgtm label Nov 9, 2024
@google-oss-prow Google OSS Prow
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acpana, maqiuyujoyce

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit d3b7408 into GoogleCloudPlatform:master Nov 9, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maqiuyujoyce maqiuyujoyce maqiuyujoyce left review comments

@acpana acpana acpana left review comments

@yuwenma yuwenma yuwenma left review comments

Assignees

@acpana acpana

@maqiuyujoyce maqiuyujoyce

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ziyue-101 @acpana @yuwenma @maqiuyujoyce